Use @this to satisfy ESLint's no-invalid-this #14406
Conversation
deyaaeldeen
requested review from
bterlson,
chradek,
daviwil,
ellismg,
jeremymeng,
joheredi,
KarishmaGhiya,
maorleger,
sadasant,
sarangan12,
schaabs,
southpolesteve,
witemple-msft,
xirzec,
zfoster and
a team
as code owners
ghost
added
Azure.Core
Cosmos
Event Grid
Azure.Identity
KeyVault
Tables
EngSys
sdk/anomalydetector/ai-anomaly-detector/test/anomalydetector.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-certificates/test/utils/lro/restore/operation.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-secrets/test/utils/lro/restore/operation.ts
Outdated
Show resolved
Hide resolved
|
/azp run js - eventgrid - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hello @ramya-rao-a! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
|
I'm a little late to the party here but I am not in favor of this approach personally for the following reasons:
I love the work you put into this, and I love that we're making our tooling better, but since we didn't solve the underlying problem what do I do when I have a linter error in tests, just tag |
|
@maorleger I feel your pain and you can see a discussion around this in the PR description. I am not really fond of either this approach or turning off the rule all together in tests. The underlying issue lies in how mocha works which is disturbing to me. I mean, look at some of the side-effects of their design: #13005. I can be ok with turning the rule off for tests but that solution still does not solve the underlying problem. Anyhow, I do not have a strong preference either way and we should not expect to use a lot of |
|
@deyaaeldeen Yeah, I think it just comes with the territory - One compromise / improvement we can make here is using typescript-eslint's So then instead of it("can abort creating a key", /** @this Mocha.Context */ async function() {we would use it("can abort creating a key", async function(this: Mocha.Context) And then at least it's just TypeScript without the inline JSDoc comments - what do you think? We already use typescript-eslint in a few places. Edit, we also get the benefit of the compiler letting us know that we cannot use the |
|
Sounds great to me! Sorry for being trigger happy and merging this PR :( |
Implements the approach described here: #14406 (comment) to linting `this` references. Reminder to myself: look for typescript-eslint alternatives when the behavior of eslint is not satisfactory.
# Problem `no-invalid-this` makes sure uses of `this` are valid (see [docs](https://eslint.org/docs/rules/no-invalid-this) and [implementation](https://github.com/eslint/eslint/blob/8984c91372e64d1e8dd2ce21b87b80977d57bff9/lib/rules/utils/ast-utils.js#L900)). However, uses of `this` are rampant in our test suites because this is how mocha unit tests are structured, the Mocha context can be accessed only through `this`. # Fix So instead of disabling `no-invalid-this` in our test suites, this PR tags functions that reference `this` with `@this` and that satisfies the rule requirements (see [docs](https://eslint.org/docs/rules/no-invalid-this)). # Discussion It could be argued that this work just replaces one comment annotation with another so we did not really solve the underlying problem. However, the inherent problem lies in how mocha itself works and there is nothing we can do other than probably migrating to another framework that is more sane/type-safe. One minor improvement we get is we now have slightly less syntactic overhead because we need to tag just the function instead of individual lines in its body that violate the rule. # Trade-offs Pros: - function tags are less than line tags Cons: - whitelisting one more tag with the tsdoc linter that our devs need to learn about - still having rampant tags everywhere Fixes Azure#11404
Implements the approach described here: Azure#14406 (comment) to linting `this` references. Reminder to myself: look for typescript-eslint alternatives when the behavior of eslint is not satisfactory.
Fix pattern of cluster name and data center name (Azure#14406) * Fix pattern of cluster name and data center name * Fix pattern of cluster name and data center name Co-authored-by: wentingwu <[email protected]>
Problem
no-invalid-thismakes sure uses ofthisare valid (see docs and implementation). However, uses ofthisare rampant in our test suites because this is how mocha unit tests are structured, the Mocha context can be accessed only throughthis.Fix
So instead of disabling
no-invalid-thisin our test suites, this PR tags functions that referencethiswith@thisand that satisfies the rule requirements (see docs).Discussion
It could be argued that this work just replaces one comment annotation with another so we did not really solve the underlying problem. However, the inherent problem lies in how mocha itself works and there is nothing we can do other than probably migrating to another framework that is more sane/type-safe. One minor improvement we get is we now have slightly less syntactic overhead because we need to tag just the function instead of individual lines in its body that violate the rule.
Trade-offs
Pros:
Cons:
Fixes #11404